Skip to content

Fix IB patch vel/angular_vel/angles broadcast inside wrong loop#1188

Closed
sbryngelson wants to merge 1 commit into
MFlowCode:masterfrom
sbryngelson:fix/ib-patch-broadcast-loop
Closed

Fix IB patch vel/angular_vel/angles broadcast inside wrong loop#1188
sbryngelson wants to merge 1 commit into
MFlowCode:masterfrom
sbryngelson:fix/ib-patch-broadcast-loop

Conversation

@sbryngelson

@sbryngelson sbryngelson commented Feb 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

Severity: HIGH — IB patch vel/angular_vel/angles broadcast in wrong loop with wrong bounds.

File: src/pre_process/m_mpi_proxy.fpp, lines 79-81

The patch_ib(i)%vel, patch_ib(i)%angular_vel, and patch_ib(i)%angles broadcasts are inside the do i = 1, num_bc_patches_max loop, but they reference patch_ib which should be iterated up to num_patches_max (where all other patch_ib fields are broadcast).

Before

do i = 1, num_bc_patches_max        ! BC patches loop
    ! patch_bc broadcasts... (correct)
    call MPI_BCAST(patch_ib(i)%vel, ...)          ! WRONG loop!
    call MPI_BCAST(patch_ib(i)%angular_vel, ...)  ! WRONG loop!
    call MPI_BCAST(patch_ib(i)%angles, ...)       ! WRONG loop!
end do
do i = 1, num_patches_max           ! patches loop
    ! other patch_ib broadcasts... (correct loop, missing vel/angular_vel/angles)
end do

After

Moved the vel/angular_vel/angles broadcasts into the num_patches_max loop alongside the other patch_ib fields.

Why this went undetected

If num_bc_patches_max >= num_patches_max, all IB patches would still get broadcast (with some extra unnecessary iterations). Only fails when there are more IB patches than BC patches.

Test plan

  • Run multi-rank simulation with immersed boundaries
  • Verify IB patch velocities/angles are consistent across ranks

🤖 Generated with Claude Code

Fixes #1208

Copilot AI review requested due to automatic review settings February 21, 2026 03:23
@codeant-ai

codeant-ai Bot commented Feb 21, 2026

Copy link
Copy Markdown
Contributor

CodeAnt AI is reviewing your PR.

Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

@coderabbitai

coderabbitai Bot commented Feb 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Warning

Rate limit exceeded

@sbryngelson has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 28 minutes and 28 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codeant-ai codeant-ai Bot added the size:XS label Feb 21, 2026
codeant-ai[bot]
codeant-ai Bot reviewed Feb 21, 2026
View reviewed changes
Comment thread src/pre_process/m_mpi_proxy.fpp
@codeant-ai

codeant-ai Bot commented Feb 21, 2026

Copy link
Copy Markdown
Contributor

Nitpicks 🔍

🔒 No security issues identified
⚡ Recommended areas for review

  • Consistency of broadcast loop
    The motion fields were moved from the boundary-patch loop to the main patch loop — this is likely correct. Still verify that num_patches_max covers all patches that need motion data and that no IB patches exist only under num_bc_patches_max such that they might be missed.

  • Hard-coded count
    The new MPI_BCAST for vel, angular_vel and angles uses a fixed count of 3. If any of these arrays are not guaranteed to be length 3 for all patches (e.g. different dimensionality or future changes), the broadcast will be incorrect. Prefer using the actual array size (size(...)) so the count always matches the local variable storage.

  • Broadcast granularity
    The code issues many small MPI_BCAST calls (per-field, per-patch). For large numbers of patches this can create significant MPI overhead and hurt scalability. Consider packing related IB fields into a single contiguous buffer (or using derived MPI types) and broadcasting once per patch to reduce message count.

@codeant-ai

codeant-ai Bot commented Feb 21, 2026

Copy link
Copy Markdown
Contributor

CodeAnt AI finished reviewing your PR.

Copilot AI reviewed Feb 21, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Moves the patch_ib broadcasts for vel, angular_vel, and angles to the correct loop so they align with the other patch_ib fields and are broadcast for the proper num_patches_max range.

Changes:

  • Removed patch_ib vector broadcasts from the patch_bc loop (num_bc_patches_max context).
  • Added patch_ib vector broadcasts alongside the other patch_ib broadcasts (num_patches_max context).

cubic-dev-ai[bot]

This comment was marked as off-topic.

Sign in to view

@sbryngelson @claude
Fix IB patch vel/angular_vel/angles broadcast inside wrong loop
7c10279 
patch_ib vel, angular_vel, and angles were broadcast inside the
num_bc_patches_max loop instead of the num_patches_max loop where
the other patch_ib fields are broadcast.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@sbryngelson sbryngelson force-pushed the fix/ib-patch-broadcast-loop branch from d291ca5 to 7c10279 Compare February 21, 2026 03:53
@sbryngelson sbryngelson mentioned this pull request Feb 21, 2026
Closed
@codecov

codecov Bot commented Feb 21, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 44.05%. Comparing base (84c46e0) to head (7c10279).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #1188   +/-   ##
=======================================
  Coverage   44.05%   44.05%           
=======================================
  Files          70       70           
  Lines       20498    20498           
  Branches     1990     1990           
=======================================
  Hits         9030     9030           
  Misses      10329    10329           
  Partials     1139     1139

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sbryngelson sbryngelson mentioned this pull request Feb 22, 2026
Merged
3 tasks
@sbryngelson

sbryngelson commented Feb 22, 2026

Copy link
Copy Markdown
Member Author

Superseded by #1241 (batched low-risk fixes)

@sbryngelson sbryngelson deleted the fix/ib-patch-broadcast-loop branch February 22, 2026 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments
+2 more reviewers
@codeant-ai codeant-ai[bot] codeant-ai[bot] left review comments
@cubic-dev-ai cubic-dev-ai[bot] cubic-dev-ai[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

IB patch vel/angular_vel/angles broadcast inside wrong loop

2 participants

@sbryngelson